Skip to content

[Bugfix][Store] Retry HA stale-handle cleanup oplog persist on transient backpressure - #29

Merged
zchuango merged 1 commit into
store/retry-ha-cleanup-oplogfrom
connor/retry-ha-cleanup-oplog
Aug 18, 2026
Merged

[Bugfix][Store] Retry HA stale-handle cleanup oplog persist on transient backpressure#29
zchuango merged 1 commit into
store/retry-ha-cleanup-oplogfrom
connor/retry-ha-cleanup-oplog

Conversation

@Connor-Matthew

@Connor-Matthew Connor-Matthew commented Aug 14, 2026

Copy link
Copy Markdown

Description

In HA mode (enable_ha_ && enable_oplog_), ClearInvalidHandles persists
stale-handle cleanup through the oplog writer before removing metadata.
Today a single transient failure makes the persist call give up
immediately:

  • TASK_PENDING_LIMIT_EXCEEDED — the batch writer's slots are momentarily
    full while it seals the current batch; capacity frees up within
    milliseconds.
  • UNAVAILABLE_IN_CURRENT_STATUS — the writer is briefly rejecting writes
    while its write_batch retries against the KV backend.

When that happens, the stale handles / last-replica metadata survive until
the next cleanup sweep, so invalid client handles linger longer than
necessary and (for the last-replica path) removed objects stay visible in
the metadata map despite the client being gone.

This PR wraps both oplog persist calls in ClearInvalidHandles with a
bounded exponential-backoff retry (RetryOplogPersist):

  • TASK_PENDING_LIMIT_EXCEEDED: retry up to 10 attempts, backoff
    min(2^attempt, 16ms) — the writer frees slots imminently, waiting pays
    off.
  • UNAVAILABLE_IN_CURRENT_STATUS: retry up to 5 attempts with the same
    backoff — recovery depends on the KV backend, so bail out earlier
    instead of spinning on a persistent outage.
  • Any other error (INVALID_PARAMS etc.): no retry, returned immediately.

Control flow is unchanged: if the persist still fails after retries,
the key is skipped exactly as before and cleanup is retried on the next
sweep. No fallback semantics are added or removed.

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Mooncake Store (mooncake-store)
  • Reshard (mooncake-reshard)
  • Mooncake EP (mooncake-ep)
  • Mooncake PG (mooncake-pg)
  • Integration (mooncake-integration)
  • P2P Store (mooncake-p2p-store)
  • Python Wheel (mooncake-wheel)
  • Common (mooncake-common)
  • Mooncake RL (mooncake-rl)
  • CI/CD
  • Docs
  • Other

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation update
  • Performance improvement
  • Other

How Has This Been Tested?

Test commands:

# Not run locally: the submitter's host is macOS, and this project can
# only be built inside a Linux container. Intended command:
ctest --test-dir build -R master_service

Test results:

  • Unit tests pass — not verified locally; relying on this PR's CI run
  • Integration tests pass (if applicable)
  • Manual testing done (describe below)

The change is a pure retry wrapper around existing calls: the success path
is byte-identical to before, and the failure path after exhausting retries
returns the same error the caller already handles (skip key, retry next
sweep). Formatting verified with clang-format 20.1.8 per
.clang-format (clean, no violations on touched lines).

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh
    (clang-format 20.1.8, no violations on touched lines)
  • I have run pre-commit on the files changed in this PR and all hooks
    pass — toolchain unavailable on the submitter's macOS host; relying
    on CI
  • I have updated the documentation (if applicable) — not applicable,
    no behavior contract change
  • I have added tests to prove my changes are effective — retry timing
    depends on the oplog writer's internal backpressure state; open to
    adding a unit test if reviewers suggest a good injection point
  • For changes >500 LOC: I have filed an RFC issue — not applicable
    (+59/-4 LOC)

AI Assistance Disclosure

  • No AI tools were used
  • AI tools were used (specify below)

Ported and adapted with AI assistance (Proma Agent): locating the upstream
call sites, adapting the retry helper to current main (the original
commits target a fork whose ClearInvalidHandles control flow has
diverged), and formatting checks. The human submitter has reviewed every
changed line and can defend the change end-to-end.

…pressure

Wrap the two oplog persist calls in ClearInvalidHandles with a bounded
exponential-backoff retry: up to 10 attempts when the batch writer's
slots are temporarily full (TASK_PENDING_LIMIT_EXCEEDED), and up to 5
attempts when the writer is retrying against the KV backend
(UNAVAILABLE_IN_CURRENT_STATUS). Non-backpressure errors and persistent
failures keep the existing behavior: the key is skipped and retried on
the next cleanup sweep.

Ported from LinQuickDev/Mooncake PR #24 (commits b9f4cee and a224d1d,
originally by Wang_YQi), adapted to current main where ClearInvalidHandles
keeps the skip-on-failure control flow.

Made-with: Proma
@zchuango
zchuango merged commit e72f726 into store/retry-ha-cleanup-oplog Aug 18, 2026
138 of 142 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants